feat(hook-store): override Hook Gas limit and tokens balances with Tenderly Simulation Data #5039

yvesfracari · 2024-10-24T14:08:32Z

Summary

Using gasUsed data of Tenderly simulation (added on bff on this PR) to override gasLimit hook data.

To Test

Run the bff server locally in this PR context
Run the app locally connecting it with your local bff
Use the Build Hook dApp to create one simple hook with a low gas limit. The data that I used to test was a pre-hook with:

Target: 0xfFf9976782d46CC05630D1f6eBAb18b2324d6B14
Gas Limit: 10
Calldata: 0x

Select two tokens go until the swap review and check the gas limit. Check if the gas limit is different from the one that you inputted and 10% bigger than the simulated one (click on the Tenderly simulation link to check the simulated)

Note: The PR is in draft until the balanceDiff PR is merged. But feel free to test it (only the diffs will have that context as well)

vercel · 2024-10-24T14:08:37Z

@yvesfracari is attempting to deploy a commit to the cow Team on Vercel.

A member of the Team first needs to authorize it.

apps/cowswap-frontend/src/modules/hooksStore/containers/HookDappContainer/index.tsx

apps/cowswap-frontend/src/modules/hooksStore/dapps/BuildHookApp/index.tsx

apps/cowswap-frontend/src/modules/hooksStore/hooks/useBalancesDiff.ts

elena-zh

All is good rn besides the issue (edge-case) we decided to address later.
Thanks!

anxolin

The description of this PR made me think the scope is smaller. I don't think the description and code changes maches. Most of this PR is about calculating the aggregated balance given the current user's balance and the result from the similation.

apps/cowswap-frontend/src/modules/combinedBalances/hooks/useTokensBalancesCombined.ts

anxolin · 2024-11-07T15:16:29Z

apps/cowswap-frontend/src/modules/hooksStore/hooks/useBalancesDiff.ts

+  const { preHooks, postHooks } = useHooks()
+  const preHookBalanceDiff = usePreHookBalanceDiff()
+
+  const orderMockBalanceDiff = useMemo(() => {


Sorry for the lack of documentation. This is used mainly when the swap isn't simulated (like if the user has only preHooks). I changed a bit the variable name and comments.

I see, thanks

apps/cowswap-frontend/src/modules/hooksStore/hooks/useBalancesDiff.ts

anxolin · 2024-11-07T15:27:14Z

apps/cowswap-frontend/src/modules/hooksStore/hooks/useBalancesDiff.ts

+  return bigA.add(bigB).toString()
+}
+
+function mergeBalanceDiffs(first: BalancesDiff, second: BalancesDiff): BalancesDiff {


Checking the type of the BalancesDiff I see is a structure to hold token balances. Having Diff in its name feels a bit constraining cause you can use it for example to hold the current balances, etc.

I would drop the diff from the name and would make the merging utilities more generic so they live not in the hookStore but in a more central place. Or at least can be moved to a utility instead of being in a hook file

I think the main difference between this and a token balances is that it can be negative. Another difference between this and the token balances used on the rest of the app is the first key. Another difference is that the outer key of BalanceDiff is the address which the difference is related to while the BalanceState of the rest of the app the outer key is the chainId since it is always related to the connected account. For now, I don't see any other place on the app where we would store balances diff from addresses, so I think that there isn't too much gain of moving this to a utility for now.

ok, makes sense

anxolin · 2024-11-07T15:28:04Z

apps/cowswap-frontend/src/modules/hooksStore/hooks/useBalancesDiff.ts

+            result[address][token] = addBigNumberStrings(result[address][token], second[address][token])
+          } catch (error) {
+            console.error(`Error adding balances for address ${address} and token ${token}:`, error)
+            throw error


Isn't this logic very similar to the other one that would apply the diffs to the current balances?

I think that is similar, however is used for only one value instead of all of them and that is why I am not importing the function from there.

apps/cowswap-frontend/src/modules/hooksStore/hooks/useHooksStateWithSimulatedGas.ts

apps/cowswap-frontend/src/modules/tenderly/utils/generateSimulationData.ts

anxolin · 2024-11-07T15:41:53Z

Gas price is overriden as described.

In this case, simulation shows 21K gas:

I can see it replaces the 10 with the 21K+10%

Are we sure we want to add the buffer? Did we encounter issues with execution failing due to gas?

yvesfracari · 2024-11-07T17:24:03Z

Thanks for the review @anxolin ! I tried to resolve / reply all your questions. About the PR size, sorry it was supposed to be two separated PRs but then it was requested to be merged and I forget to update the description.

About the gas limit, I already faced some issues of hooks not being executed due to out of gas, but never when the gas was defined by the tenderly. About the 10%, I just keep an old logic that existed on other hooks.

…e-it-with-tenderly-simulation

yvesfracari · 2024-11-07T17:59:33Z

As discussed on the channel, I removed the buffer of the Tenderly simulation gas.

github-actions · 2024-11-07T20:38:04Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

yvesfracari · 2024-11-08T15:05:52Z

1 out of 2 committers have signed the CLA.✅ @yvesfracari❌ @pedro YvesPedro Yves seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.You can retrigger this bot by commenting recheck in this Pull Request

This was due some commits with a wrong author, already fixed it. Please ignore it 😅

elena-zh · 2024-11-11T09:18:02Z

Changes LGTM.
Not for the current PR, but for the future updates, it would be nice to have one generic 're-run simulation' button that will retrigger bundle on click instead of having several ones with the same action :) Something link this:

@fairlighteth, WDYT? :)

anxolin · 2024-11-11T10:13:38Z

@yvesfracari I think you have some lint issues:

anxolin · 2024-11-11T10:20:05Z

I want to merge this PR so we start to consolidate things. Any enhancement we can try to do iterativelly to not keep this one open for too long

For this, I created this PR which applies the changes of this PR plus fixes the lint issues I posted above #5080

$yvesfracari$

yvesfracari added 23 commits October 2, 2024 11:06

$@yvesfracari$

chore: init tenderly module

3ac32e9

$@yvesfracari$

feat: enable bundle simulations

2260a54

$@yvesfracari$

refactor: change bundle simulation to use SWR

e1b5c4c

$@yvesfracari$

chore: consider custom recipient

4bae430

$@yvesfracari$

chore: merge into develop

be46417

$@yvesfracari$

chore: remove goldrush sdk

8bed508

$@yvesfracari$

fix: error on post hooks get simulation

12e0ee5

$@yvesfracari$

refactor: use bff on bundle simulation feature

450e330

$@yvesfracari$

chore: merge into develop

495a56b

$@yvesfracari$

chore: remove console.logs

7eff54d

$@yvesfracari$

chore: fix leandro comments

beccb79

$@yvesfracari$

chore: remove unused tenderly consts

0d3254c

$@yvesfracari$

refactor: rename tenderly simulation hook

2726bb1

$@yvesfracari$

chore: refactor top token holder swr to jotai with cache

6ea9624

$@yvesfracari$

chore: merge into develop

732ecc9

$@yvesfracari$

chore: rename hook to match file name

5cb5b72

$@yvesfracari$

refactor: use seconds for cache time in toptokenholder state

22501d4

$@yvesfracari$

chore: merge into develop

c80d504

$@yvesfracari$

chore: create hooks to use token balances with pre hooks

b8bb1f4

$@yvesfracari$

chore: use combined hooks on swap components

f9ade14

$@yvesfracari$

feat: expose balance diff to hook dapp context

5c3f35e

$@yvesfracari$

chore: add testing console log for balances diff access on hook dapp

5667d1f

$@yvesfracari$

feat: use tenderly simulation gas on hook creation

aab2974